Fix x-spring-provide-args handling. Pass arguments to delegates. Use JavaParser. - #24071
Conversation
80ccb69 to
c3766cd
Compare
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
please resolve the merge conflicts when you've time |
Resolved. |
c75169d to
80f02f7
Compare
|
I have identified some issues with the patch related to comma handling. I am working on a fix. |
80f02f7 to
b85a6a1
Compare
|
I think this is ready. |
b85a6a1 to
694ca42
Compare
|
Rebased. |
694ca42 to
3a6d6c9
Compare
3a6d6c9 to
68697cf
Compare
|
Rebased. |
|
@wing328 Is there a chance this can be included in the next release? |
|
thanks for the PR cc Spring technical committee: @cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @martin-mfg (2023/08) @KannaKim (2026/07) to see if they've any question/feedback. |
|
Is there anything I can do to push this through? The bug is real and this change fixes it. |
There was a problem hiding this comment.
5 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringCodegen.java:1593">
P2: When a provided parameter has a type-use annotation, this only clears annotations attached directly to `Parameter`, so the delegate still receives nested annotations. Remove annotations recursively from the cloned parameter type so delegate signatures honor the annotation-stripping behavior.</violation>
<violation number="2" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringCodegen.java:1610">
P3: If one `x-spring-provide-args` list item contains several comma-separated parameters, only the first is used and the remaining ones are silently dropped. The removed regex loop iterated `matcher.find()` over the whole string and produced one entry per parameter, so existing specs that pack multiple declarations into a single list item generated all of them (previously without commas, i.e. broken Java, now silently missing). The JavaParser-based path parses the string as one method and returns only `getParameter(0)`, so the extra declarations vanish without any warning or generation error, producing code that fails to compile with an unrelated-looking message. Validate that exactly one parameter was parsed and fail loudly (or split on top-level commas) instead of silently discarding input.</violation>
</file>
<file name="modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java">
<violation number="1" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java:8591">
P3: The four tests between `shouldPassXSpringProvideArgsToOverridableMethodWithApiInterfaceRequestMapping` and `shouldIncludeXSpringProvideArgsWithInterfaceOnlyWithoutDelegatePattern`, plus `shouldSeparatePageableAndXSpringProvideArgsInReactiveControllerImplementation`, replicate the same ~25-line setup (temp dir, `parseFlattenSpec`, `ClientOptInput`, `DefaultGenerator` flags, `generator.opts(input).generate()`) verbatim, and two of them inline byte-identical anonymous `SpringCodegen` subclasses. The class already has the `generateFromContract(String, String, Map<String, Object>, Consumer<CodegenConfigurator>)` helper that performs this exact setup and accepts additional properties (including `REQUEST_MAPPING_OPTION`, which SpringCodegen.processOpts reads via `convertPropertyToTypeAndWriteBack`), so only the `_api_controller_impl_` hook really needs the custom subclass. Extract one small static codegen factory and route the api-interface tests through `generateFromContract`; this also keeps the new tests consistent with the rest of the file.</violation>
<violation number="2" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java:8622">
P3: This assertion hard-codes a double space (`default ResponseEntity<Void> foo(...)`) into the expected output. The generated interface really does contain that double space: `api.mustache` renders the "Override this method" block as `{{#jdk8-default-interface}}default {{/jdk8-default-interface}} {{>responseType}}`, where the stray space after the section close produces two spaces between `default` and the return type — unlike the `_foo` method on the same interface (same test asserts single space) and the delegate template, both of which render single-spaced. Fix the template spacing (drop the space after `{{/jdk8-default-interface}}`) and assert the canonical `default ResponseEntity<Void>` form, so the test catches the formatting defect instead of pinning it.</violation>
<violation number="3" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java:8842">
P3: This data-driven test for api_interface mode only asserts the call text in `FooApi.java` (`return findFoo(pageable, providedArg);` etc.) plus guard strings. It never verifies the `_findFoo` signature, the `@Parameter(hidden = true) @Size(...)` annotations, or the delegate method signature — the sibling test `shouldSeparatePageableAndXSpringProvideArgsForRequestContextAndReactiveCombinations` verifies the delegate signature via `expectedApiDelegateMethodSignature(...)` but has no api_interface row. So a regression in api_interface signature/ordering/annotation output would pass this test silently. Assert the delegate signature for each row, mirroring the sibling test.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| formattedArgs.add(newArg); | ||
|
|
||
| Parameter delegateParameter = parameter.clone(); | ||
| delegateParameter.getAnnotations().clear(); |
There was a problem hiding this comment.
P2: When a provided parameter has a type-use annotation, this only clears annotations attached directly to Parameter, so the delegate still receives nested annotations. Remove annotations recursively from the cloned parameter type so delegate signatures honor the annotation-stripping behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringCodegen.java, line 1593:
<comment>When a provided parameter has a type-use annotation, this only clears annotations attached directly to `Parameter`, so the delegate still receives nested annotations. Remove annotations recursively from the cloned parameter type so delegate signatures honor the annotation-stripping behavior.</comment>
<file context>
@@ -1559,41 +1571,82 @@ public CodegenOperation fromOperation(String path, String httpMethod, Operation
formattedArgs.add(newArg);
+
+ Parameter delegateParameter = parameter.clone();
+ delegateParameter.getAnnotations().clear();
+ formattedDelegateArgs.add(delegateParameter.toString());
+ formattedArgNames.add(parameter.getNameAsString());
</file context>
| delegateParameter.getAnnotations().clear(); | |
| delegateParameter.findAll(AnnotationExpr.class).forEach(annotation -> annotation.remove()); |
| } | ||
|
|
||
| @Test | ||
| public void shouldPassXSpringProvideArgsToOverridableMethodWithApiInterfaceRequestMapping() throws IOException { |
There was a problem hiding this comment.
P3: The four tests between shouldPassXSpringProvideArgsToOverridableMethodWithApiInterfaceRequestMapping and shouldIncludeXSpringProvideArgsWithInterfaceOnlyWithoutDelegatePattern, plus shouldSeparatePageableAndXSpringProvideArgsInReactiveControllerImplementation, replicate the same ~25-line setup (temp dir, parseFlattenSpec, ClientOptInput, DefaultGenerator flags, generator.opts(input).generate()) verbatim, and two of them inline byte-identical anonymous SpringCodegen subclasses. The class already has the generateFromContract(String, String, Map<String, Object>, Consumer<CodegenConfigurator>) helper that performs this exact setup and accepts additional properties (including REQUEST_MAPPING_OPTION, which SpringCodegen.processOpts reads via convertPropertyToTypeAndWriteBack), so only the _api_controller_impl_ hook really needs the custom subclass. Extract one small static codegen factory and route the api-interface tests through generateFromContract; this also keeps the new tests consistent with the rest of the file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java, line 8591:
<comment>The four tests between `shouldPassXSpringProvideArgsToOverridableMethodWithApiInterfaceRequestMapping` and `shouldIncludeXSpringProvideArgsWithInterfaceOnlyWithoutDelegatePattern`, plus `shouldSeparatePageableAndXSpringProvideArgsInReactiveControllerImplementation`, replicate the same ~25-line setup (temp dir, `parseFlattenSpec`, `ClientOptInput`, `DefaultGenerator` flags, `generator.opts(input).generate()`) verbatim, and two of them inline byte-identical anonymous `SpringCodegen` subclasses. The class already has the `generateFromContract(String, String, Map<String, Object>, Consumer<CodegenConfigurator>)` helper that performs this exact setup and accepts additional properties (including `REQUEST_MAPPING_OPTION`, which SpringCodegen.processOpts reads via `convertPropertyToTypeAndWriteBack`), so only the `_api_controller_impl_` hook really needs the custom subclass. Extract one small static codegen factory and route the api-interface tests through `generateFromContract`; this also keeps the new tests consistent with the rest of the file.</comment>
<file context>
@@ -8585,6 +8587,308 @@ void schemaMappingWithNullableAllOfRendersNullableJavaProperty() throws IOExcept
}
+ @Test
+ public void shouldPassXSpringProvideArgsToOverridableMethodWithApiInterfaceRequestMapping() throws IOException {
+ File output = Files.createTempDirectory("test").toFile().getCanonicalFile();
+ output.deleteOnExit();
</file context>
| INCLUDE_HTTP_REQUEST_CONTEXT, Boolean.toString(includeHttpRequestContext))); | ||
|
|
||
| JavaFileAssert.assertThat(files.get("FooApi.java")) | ||
| .fileContains(expectedOverrideCall) |
There was a problem hiding this comment.
P3: This data-driven test for api_interface mode only asserts the call text in FooApi.java (return findFoo(pageable, providedArg); etc.) plus guard strings. It never verifies the _findFoo signature, the @Parameter(hidden = true) @Size(...) annotations, or the delegate method signature — the sibling test shouldSeparatePageableAndXSpringProvideArgsForRequestContextAndReactiveCombinations verifies the delegate signature via expectedApiDelegateMethodSignature(...) but has no api_interface row. So a regression in api_interface signature/ordering/annotation output would pass this test silently. Assert the delegate signature for each row, mirroring the sibling test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java, line 8842:
<comment>This data-driven test for api_interface mode only asserts the call text in `FooApi.java` (`return findFoo(pageable, providedArg);` etc.) plus guard strings. It never verifies the `_findFoo` signature, the `@Parameter(hidden = true) @Size(...)` annotations, or the delegate method signature — the sibling test `shouldSeparatePageableAndXSpringProvideArgsForRequestContextAndReactiveCombinations` verifies the delegate signature via `expectedApiDelegateMethodSignature(...)` but has no api_interface row. So a regression in api_interface signature/ordering/annotation output would pass this test silently. Assert the delegate signature for each row, mirroring the sibling test.</comment>
<file context>
@@ -8585,6 +8587,308 @@ void schemaMappingWithNullableAllOfRendersNullableJavaProperty() throws IOExcept
+ INCLUDE_HTTP_REQUEST_CONTEXT, Boolean.toString(includeHttpRequestContext)));
+
+ JavaFileAssert.assertThat(files.get("FooApi.java"))
+ .fileContains(expectedOverrideCall)
+ .fileDoesNotContain("findFoo(,", "servletRequestpageable", "exchangepageable");
+ }
</file context>
| .fileContains("default ResponseEntity<Void> _foo(") | ||
| .fileContains("@Parameter(hidden = true) @Size(max = 64) String providedArg") | ||
| .fileContains("return foo(providedArg);") | ||
| .fileContains("default ResponseEntity<Void> foo(String providedArg)"); |
There was a problem hiding this comment.
P3: This assertion hard-codes a double space (default ResponseEntity<Void> foo(...)) into the expected output. The generated interface really does contain that double space: api.mustache renders the "Override this method" block as {{#jdk8-default-interface}}default {{/jdk8-default-interface}} {{>responseType}}, where the stray space after the section close produces two spaces between default and the return type — unlike the _foo method on the same interface (same test asserts single space) and the delegate template, both of which render single-spaced. Fix the template spacing (drop the space after {{/jdk8-default-interface}}) and assert the canonical default ResponseEntity<Void> form, so the test catches the formatting defect instead of pinning it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java, line 8622:
<comment>This assertion hard-codes a double space (`default ResponseEntity<Void> foo(...)`) into the expected output. The generated interface really does contain that double space: `api.mustache` renders the "Override this method" block as `{{#jdk8-default-interface}}default {{/jdk8-default-interface}} {{>responseType}}`, where the stray space after the section close produces two spaces between `default` and the return type — unlike the `_foo` method on the same interface (same test asserts single space) and the delegate template, both of which render single-spaced. Fix the template spacing (drop the space after `{{/jdk8-default-interface}}`) and assert the canonical `default ResponseEntity<Void>` form, so the test catches the formatting defect instead of pinning it.</comment>
<file context>
@@ -8585,6 +8587,308 @@ void schemaMappingWithNullableAllOfRendersNullableJavaProperty() throws IOExcept
+ .fileContains("default ResponseEntity<Void> _foo(")
+ .fileContains("@Parameter(hidden = true) @Size(max = 64) String providedArg")
+ .fileContains("return foo(providedArg);")
+ .fileContains("default ResponseEntity<Void> foo(String providedArg)");
+ }
+
</file context>
| CompilationUnit compilationUnit = StaticJavaParser.parse(String.format(Locale.ROOT, "class Dummy { void method(%s) {} }", oneArg)); | ||
| return compilationUnit.findFirst(MethodDeclaration.class) | ||
| .orElseThrow(() -> new IllegalArgumentException("Unable to parse x-spring-provide-args parameter: " + oneArg)) | ||
| .getParameter(0); |
There was a problem hiding this comment.
P3: If one x-spring-provide-args list item contains several comma-separated parameters, only the first is used and the remaining ones are silently dropped. The removed regex loop iterated matcher.find() over the whole string and produced one entry per parameter, so existing specs that pack multiple declarations into a single list item generated all of them (previously without commas, i.e. broken Java, now silently missing). The JavaParser-based path parses the string as one method and returns only getParameter(0), so the extra declarations vanish without any warning or generation error, producing code that fails to compile with an unrelated-looking message. Validate that exactly one parameter was parsed and fail loudly (or split on top-level commas) instead of silently discarding input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringCodegen.java, line 1610:
<comment>If one `x-spring-provide-args` list item contains several comma-separated parameters, only the first is used and the remaining ones are silently dropped. The removed regex loop iterated `matcher.find()` over the whole string and produced one entry per parameter, so existing specs that pack multiple declarations into a single list item generated all of them (previously without commas, i.e. broken Java, now silently missing). The JavaParser-based path parses the string as one method and returns only `getParameter(0)`, so the extra declarations vanish without any warning or generation error, producing code that fails to compile with an unrelated-looking message. Validate that exactly one parameter was parsed and fail loudly (or split on top-level commas) instead of silently discarding input.</comment>
<file context>
@@ -1559,41 +1571,82 @@ public CodegenOperation fromOperation(String path, String httpMethod, Operation
+ CompilationUnit compilationUnit = StaticJavaParser.parse(String.format(Locale.ROOT, "class Dummy { void method(%s) {} }", oneArg));
+ return compilationUnit.findFirst(MethodDeclaration.class)
+ .orElseThrow(() -> new IllegalArgumentException("Unable to parse x-spring-provide-args parameter: " + oneArg))
+ .getParameter(0);
+ }
+
</file context>
|
tested locally to confirm the fix thanks for the fix and sorry for taking so long to get it merged since there are too many PRs targeting this repo. |
This fixes #24063. This is an alternative to #24064. This utilizes JavaParser instead of regular expressions to extract the information it needs.
Summary by cubic
Fixes x-spring-provide-args: replaces regex parsing with JavaParser and forwards provided args across API interfaces, controllers, and delegates in all mapping modes. Previously, provided args were not reliably parsed or passed to delegates; now they are parsed as Java parameters, imports are generated, and signatures/ordering are consistent.
Parse with
com.github.javaparserand auto-import; simplify fully qualified annotation and generic type names (incl. nested generics).Emit annotated parameters for interfaces/controllers; strip annotations in delegates. Add vendor extensions
springProvideArgsNamesandspringProvideArgsDelegate; updateapi.mustache,apiController.mustache, andapiDelegate.mustacheto pass args by name, includingapi_interfaceand interface-only modes.Preserve Pageable position and fix commas/ordering with request context, reactive, and provided args. Add tests covering with/without provide-args, pageable, reactive, controller-to-delegate, and
api_interface.Dependencies: move
com.github.javaparser:javaparser-coreto compile scope.Written for commit 68697cf. Summary will update on new commits.